-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Integrate libc-test cases into the build system #25270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
zig build test-libc -Dlibc-test-path=/path/to/libc-test
cases.addLibcTestCase("functional/pthread_tsd.c", false, .{}); | ||
cases.addLibcTestCase("functional/qsort.c", true, .{}); | ||
cases.addLibcTestCase("functional/random.c", true, .{}); | ||
cases.addLibcTestCase("functional/search_hsearch.c", false, .{}); // The test suite of wasi-libc runs this test case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only non-pthread test included in the tests of wasi-libc, which fails with zig. It seems to be caused by a difference in memory allocation:
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
int main(void) {
printf("%p, %s\n", malloc(SIZE_MAX), strerror(errno));
}
$ clang --target=wasm32-wasi --sysroot=/path/to/wasi-libc/sysroot test.c -c -fno-builtin
$ clang --target=wasm32-wasi --sysroot=/path/to/wasi-libc/sysroot -resource-dir /path/to/wasi-libc/build/wasm32-wasi/resource-dir test.o -fno-builtin -o compiled_with_clang
$ zig build-exe -target wasm32-wasi-musl test.c -lc -cflags -fno-builtin -- -femit-bin=compiled_with_zig
$ wasmtime compiled_with_clang
0, Out of memory
$ wasmtime compiled_with_zig
0, Success
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's pretty odd... I don't think we do anything differently from upstream wasi-libc with regards to malloc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we do actually! wasi-libc defaults to dlmalloc, but we default to emmalloc. That might explain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jedisct1 do you know why we do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emmalloc has better performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this is missing the dlopen tests. It’s possible to run them using the Zig build system: rpkak/zig-libc-test#1
Also, it may be worthwhile to run the tests with glibc and maybe even Darwin libc.
@@ -0,0 +1,143 @@ | |||
pub fn addCases(cases: *tests.LibcContext) void { | |||
cases.addLibcTestCase("api/main.c", true, .{}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This doesn’t actually run any of the api tests. In the original Makefile build, main
is compiled by including all of the object files under src/api
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The api
tests make sure that the libc headers are correct. I did not include the tests, since I don't think it's planned to modify the headers (@alexrp correct me if that's wrong).
I included the main.c
test to have one test, which does not depend on the used functions but signals bigger errors like bugs in the C startup code on failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
api
tests make sure that the libc headers are correct. I did not include the tests, since I don't think it's planned to modify the headers (@alexrp correct me if that's wrong).
That's right. The goal for libzigc is to be API/ABI compatible with established libcs (at least when targeting them).
I included the
main.c
test to have one test, which does not depend on the used functions but signals bigger errors like bugs in the C startup code on failure.
I imagine our regular module tests would already catch this. But I also don't have any particular objection to including this test either.
The reason for this PR is to make it easier to test the libc implementations, which Zig uses for static compilation, while these get rewritten to Zig (#2879). These libc implementations, which Zig uses for static compilation, do not support the dlopen function (see here). Maybe libzigc will implement it in the future, in which case the dlopen test cases can be added. |
I can't think of a good reason to do that off the top of my head at least. Maybe if we were concerned that the tests themselves are wrong, but I don't think we have any reason to believe that; AFAIK libc-test is considered pretty high quality, to the point that it's uncovered real flaws in other libcs. |
Sounds good. I guess I was too preoccupied with whether we could run the tests and not if we should 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
I ran this on my machine against tip of libc-test (commit f2bac7711bec93467b73bec1465579ea0b8d5071) and everything passed cleanly with -fqemu
and -fwasmtime
.
Build script changes look great.
Oh yeah, forgot to add - let's be good open source citizens about this and contribute back to libc-test provided that Szabolcs Nagy welcomes those contributions. |
I see this failure on every target:
This is a 192 GB system, so it's definitely not actually hitting OOM. Seems to work fine other than that particular test though. |
You can run the libc-test cases with:
/path/to/libc-test
is the path to a clone ofgit://nsz.repo.hu:49100/repo/libc-test
(repo website).I originally wanted to include windows targets, but I decided against it, because too many test cases seemed linux dependent. I instead suggest integrating test cases of mingw into the build system in a follow-up PR.
The test suite of wasi-libc contains tests from libc-test (included in this PR) and other tests (maybe in a follow-up PR).
libc-test also includes math tests, but I don't include them in this PR, because these tests have many target-dependent errors and I don't know who is responsible for them (maybe libc-test itself, maybe musl, maybe qemu, maybe the compiler).
Some other tests are also removed. These are failing tests, which I don't think should be fixed (e.g. strptime which includes glibc specific tests).
Some targets (mips, hexagon) are commented out. Some test cases that pass on other targets fail on these targets; maybe a bug in qemu.
This is the first step of #23538.